Raise on evaluate_js failure instead of returning a null DataSpace - #905

Summary

execute(type="evaluate_js") failed silently, while scrape() — the same contract, in the same file — got it right.

Both eval-js failure branches in NotteSession._aexecute_impl set success = False and a descriptive message, but left exception = None. Both raise gates key off exception, not success:

• notte-browser/src/notte_browser/session.py: if _raise_on_failure and exception is not None
• notte-sdk/src/notte_sdk/endpoints/sessions.py: if _raise_on_failure and result.exception is not None

raise_on_session_execution_failure = true is the shipped default, so it never fired. The caller got success=False, data=None, exception=None and crashed two lines later on result.data.markdown with 'NoneType' object has no attribute 'markdown', with the real reason ("JavaScript evaluation timed out after 45000ms") sitting unread in .message. Five deployed marketplace Functions failed this way; one turned "the page never loaded" into a reported JSON parse error.

scrape() already does the right thing: it records the failure to the trajectory with exception=e and re-raises unless raise_on_failure=False, in which case it returns a value that says it failed rather than None. This makes eval-js behave the same way.

What changed

1. Raise on evaluate_js failure instead of returning a null DataSpace

• attach an ActionExecutionError carrying message as its reason on both the asyncio.TimeoutError and PlaywrightError branches, the way controller.py already does for "Element is disabled"
• extend the SDK's existing generic-message fallback to recognise ActionExecutionError's user-facing message, so a remote caller is raised the actual reason instead of "Sorry, this action cannot be executed at the moment."

The success path is untouched. A JS null is still a successful evaluation returning data.markdown == "null", which is what makes result.data is None after an eval an unambiguous failure signal.

2. Gate raise_on_failure on failure, not on an exception having been thrown — ⚠️ behaviour change, droppable

Kept as a separate commit so it can be dropped in review without losing the eval-js fix.

Both gates now read not success instead of exception is not None, synthesising an ActionExecutionError from .message when no exception is available. This is a real behaviour change for external callers who today receive a silent success=False — most relevantly anyone with a custom BaseTool whose ExecutionResult carries success=False, which will now raise under the default.

Evidence it is safe in-repo — every consumer that wants quiet failures already opts out explicitly:

• notte-agent/src/notte_agent/agent.py:251 — raise_on_failure=False
• notte-agent/src/notte_agent/agent_fallback.py:127 — raise_on_failure=False (and it rejects raise_on_failure=True outright at line 104)
• notte-mcp/src/notte_mcp/server.py:138 — builds its session with raise_on_failure=False
• the documented pattern for optional actions is already raise_on_failure=False (docs/src/guides/web_automation_tips.mdx, browser-controls/{conditional_actions,error_handling}.mdx, guides/handle_optional_popup.mdx)

And the practical blast radius is narrower than it looks: controller.execute() returns a bool, but it raises on interaction failures (ActionExecutionError, InvalidActionError, FailedToUploadFileError, …) rather than returning False — the only False it returns is for HelpAction, which is agent-only. So a failed click already raises today. No in-repo tool returns success=False. The paths this commit actually newly covers are third-party tools and HelpAction.

3. Document raise_on_failure's actual default — docs/src/features/sessions/configuration.mdx claimed default={false}; notte-core/src/notte_core/config.toml sets it to true.

The remote path

Catalog Functions run against the API, so the action executes server-side and the failure has to survive serialisation. Verified by round-tripping a real ExecutionResult through model_dump_json() / model_validate_json():

• ExecutionResult.exception serialises via json_encoders to str(e), and the field_validator rebuilds it as a NotteBaseError. The concrete type is not preserved, so a remote caller can never receive ActionExecutionError itself — only a NotteBaseError.
• str(e) is the message captured at construction time, i.e. whatever ErrorConfig mode the API is in. The existing _GENERIC_UNEXPECTED_MESSAGES entries are verbatim user_message strings from notte_browser/errors.py, which is good evidence the API serialises in user mode. In that mode ActionExecutionError reduces to "Sorry, this action cannot be executed at the moment. …" and the reason is lost — hence the prefix check added in commit 1. tests/sdk/test_execute_raise_on_failure.py covers both developer and user server modes and fails on the user case without it.
• Commit 2's SDK-side gate additionally covers version skew: until the API ships this fix it will keep returning success=False, exception=None, and the client raises the reason from .message anyway.

Tests

tests/test_session.py (local) and tests/sdk/test_execute_raise_on_failure.py (remote, new file):

• eval-js timeout raises under the default; Playwright error raises under the default
• raise_on_failure=False still returns a result that says it failed — success=False, message intact, exception set — and does not regress to returning None
• success path untouched, including a JS null → data.markdown == "null", success=True, exception is None
• an action that fails by returning False raises under the default and stays quiet with raise_on_failure=False
• remote: raise survives the JSON round trip in both developer and user server error modes, and when the server attaches no exception at all

Ran:

• uv run pytest tests/test_session.py -q — 33 passed, 1 skipped, 1 failed: test_step_should_return_valid_timed_span, which calls Gemini and fails with key=None in a worktree with no .env. It passes in the main checkout, and it fails identically with these commits stashed.
• uv run pytest tests/sdk/test_execute_raise_on_failure.py -q — 5 passed
• uv run pytest tests/browser tests/test_trajectory.py tests/actions tests/mcp tests/code tests/config -q — 173 passed, 7 skipped. The 2 failures + 2 errors are all missing-credentials or network flakiness (test_tools.py needs a real NOTTE_API_KEY; test_screenshot_types.py depends on google.com's live DOM and fails identically with these commits stashed).
• Each new failure-path test was confirmed to fail against the unpatched source before being confirmed to pass against the patched source.
• pre-commit on every commit: ruff check, ruff format, basedpyright (0 errors, 0 warnings), detect-secrets, forbidden/playwright import checks, docs link checks — all pass. The docs-sdk-generate hook was skipped: it re-fetches https://api.notte.cc/openapi.json and rewrites docs/src/llms.txt with unrelated live-API drift (mailboxes, profile-duplicate, …). No public signature or docstring changed, so no reference doc regeneration is warranted.

Not run: integration suites requiring API credentials (tests/integration/**), and no marketplace sweeps.

🤖 Generated with Claude Code

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

Summary by CodeRabbit

• New Features

  • Session actions now raise clear errors for timeouts, browser failures, JavaScript evaluation failures, and other unsuccessful results by default.
  • Failure messages preserve the original reason, including when no exception is provided.

• Configuration

  • raise_on_failure now defaults to enabled.
  • Disable this option to receive failed results without raising an error.

• Bug Fixes

  • Improved consistency between local and remote session failure handling.
  • Actions returning unsuccessful results are now handled consistently across execution types.
  • JavaScript failures now report the underlying evaluation error instead of misleading secondary errors.

Added: the generated code, as a test

The unit tests above cover the two failure branches. This covers the thing the branches exist for — the pattern the anything-api builder actually emitted, reduced from the real tapology.com, pokerdb.thehendonmob.com and carrefour.be sources:

extraction = session.execute(type="evaluate_js", code=BOUT_SEARCH_SCRIPT)
raw_payload = extraction.data.markdown          # ← the line five Functions died on

tests/integration/test_generated_function_patterns.py, which the CI suite already picks up (only test_webvoyager_resolution and test_e2e are excluded). Runs in ~9s against example.com, the page the other integration tests here use.

The failure is provoked the way it happened in production rather than synthetically: a script that reads a property off an element it expects, run against a page that does not have it. That is what a block page, an interstitial or a redesign looks like from inside evaluate_js, and it is how at least three of the five actually failed.

Checked against the code before this branch, the first test reports:

Tapology bout search request failed: 'NoneType' object has no attribute 'markdown'

which is verbatim what the catalogue ledger recorded for that Function in production. With the branch, the same call reports JavaScript evaluation failed: ... and names the page.

Three cases:

The third is a characterisation test, not a regression test, and is included because it pins the path the builder prompt now teaches callers to take. Calling that out so nobody reads three green ticks as three guarantees.

Suite: 36 passed. One unrelated pre-existing failure, test_step_should_return_valid_timed_span, which makes a live LLM call and fails on credentials in a worktree — it fails identically without these commits.